I forgot to create a separate branch, so this PR contains two different fixes.

The "better check for ID" changes to utils.js actually fixes a different problem. The previous check for id was causing corrupted hook arguments (incorrect number of arguments being passed from one hook to another) and was crashing the server. I was running into issues only during accidental, malformed queries from the browser while using canjs-feathers. This check makes sure that the same number of hook arguments are passed between hooks even when the query is malformed. This way also allows the service the opportunity to handle the malformed query instead of throwing an unhandled error inside a hook.

In my testing, the afterHooks are executing in the correct order. It's only the beforeHooks that were executing in reverse order.

I think this might be due to the way it is mixed in. I think I ran into the execution order problem before but didn't dig into it more. I will add some tests for your PR and then merge it and make a new release.

I'll test this more later today, but I'm pretty sure that this only fixes execution order when using array syntax, but the problem still exists for "Dynamic Registrations":

userService.before({
   GROUP 1
});
userService.before({
   GROUP 2
});

I believe that the hooks in GROUP 2 will be run before those in GROUP 1, still. It would make sense to me that those declared first should be executed first.

I also did some tests and something like

service.before({
  get: function(hook, next) {
    hook.params.items = ['first'];
    next();
  }
});

service.before({
  get: [
    function(hook, next) {
      hook.params.items.push('second');
      next();
    },
    function(hook, next) {
      hook.params.items.push('third');
      next();
    }
  ]
});

Does not work. The reason is that hooks are mixed in individually so for the second .before the old .get looks like the original method which it then hooks into which is why they get reversed. Fixing this the right way might take a little more of a reorganization namely having the hooks not mixed in individually but run at once in the correct order.

It seems like it works in my apps, but I am passing the hook into next

next(null, hook)

Does that make a difference for your test?

—
Sent from Mailbox

On Sun, Dec 28, 2014 at 10:31 AM, David Luecke [email protected]
wrote:

I also did some tests and something like

service.before({
  get: function(hook, next) {
    hook.params.items = ['first'];
    next();
  }
});
service.before({
  get: [
    function(hook, next) {
      hook.params.items.push('second');
      next();
    },
    function(hook, next) {
      hook.params.items.push('third');
      next();
    }
  ]
});

Does not work. The reason is that hooks are mixed in individually so for the second .before the old .get looks like the original method which it then hooks into which is why they get reversed. Fixing this the right way might take a little more of a reorganization namely having the hooks not mixed in individually but run at once in the correct order.

Reply to this email directly or view it on GitHub:
#14 (comment)

Or I might just be adding to hooks.data, if that makes a difference.

—
Sent from Mailbox

On Sun, Dec 28, 2014 at 10:31 AM, David Luecke [email protected]
wrote:

I also did some tests and something like

service.before({
  get: function(hook, next) {
    hook.params.items = ['first'];
    next();
  }
});
service.before({
  get: [
    function(hook, next) {
      hook.params.items.push('second');
      next();
    },
    function(hook, next) {
      hook.params.items.push('third');
      next();
    }
  ]
});

Does not work. The reason is that hooks are mixed in individually so for the second .before the old .get looks like the original method which it then hooks into which is why they get reversed. Fixing this the right way might take a little more of a reorganization namely having the hooks not mixed in individually but run at once in the correct order.

Reply to this email directly or view it on GitHub:
#14 (comment)

Oh. I think I misunderstood.  I thought you were saying that the data wasn't getting passed between hooks.

I thought that the hooks were set up in a chain.  That's how I did things in feathers-mongoose-advanced.  I didn't realize they were individually hooked up.

—
Sent from Mailbox

On Sun, Dec 28, 2014 at 10:31 AM, David Luecke [email protected]
wrote:

I also did some tests and something like

service.before({
  get: function(hook, next) {
    hook.params.items = ['first'];
    next();
  }
});
service.before({
  get: [
    function(hook, next) {
      hook.params.items.push('second');
      next();
    },
    function(hook, next) {
      hook.params.items.push('third');
      next();
    }
  ]
});

Does not work. The reason is that hooks are mixed in individually so for the second .before the old .get looks like the original method which it then hooks into which is why they get reversed. Fixing this the right way might take a little more of a reorganization namely having the hooks not mixed in individually but run at once in the correct order.

Reply to this email directly or view it on GitHub:
#14 (comment)

Well it kind of is a chain but it gets reversed (which is indeed not what you'd expect it to do). Basically it turns the hooks into Uberproto mixins but when overriding a function multiple times the last one will get executed first. The solution is probably storing the hook objects on the service and running them in the correct order instead of mixing them in individually. I'll have a look and will probably include #11 as well.

@daffl yes I think you are right. It's probably better to just store the hooks and then call them in the order they registered, rather than mixing them in. Once the hooks are finished then just call the original method.

This has been fixed via #16